Fix integer underflow in Chakra napi_get_value_string_* zero-bufsize handling#197
Open
bkaradzic-microsoft wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the Chakra Node-API string getter implementations against bufsize == 0 with a non-null buffer to prevent size_t underflow and out-of-bounds writes, and adds a native regression test to validate the behavior.
Changes:
- Add
bufsize == 0short-circuit handling in Chakranapi_get_value_string_latin1/utf8to avoid an OOB terminator write and avoid the slow-path allocation. - Gate Chakra
napi_get_value_string_utf16copy/terminator logic onbufsize != 0to preventbufsize - 1underflow. - Add a native unit test covering the
napi_get_value_string_utf16(buf != nullptr, bufsize == 0)regression scenario.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Tests/UnitTests/Shared/Shared.cpp | Adds a native regression test for napi_get_value_string_utf16 zero-bufsize behavior (currently built on all non-JSI backends). |
| Core/Node-API/Source/js_native_api_chakra.cc | Fixes Chakra string getters’ handling of (buf != nullptr, bufsize == 0) to prevent underflow/OOB writes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+354
to
+356
| // The V8JSI Node-API shim does not expose napi_get_value_string_utf16, so this | ||
| // native test only builds on the Chakra, V8, and JavaScriptCore backends. | ||
| #if !defined(JSRUNTIMEHOST_NAPI_ENGINE_JSI) |
Member
Author
There was a problem hiding this comment.
Good catch — JavaScriptCore is vulnerable too (its JSString::CopyTo* helpers do the same bufsize - 1 underflow and buf[size] = 0). Rather than guarding the test off JSC, I extended the fix: js_native_api_javascriptcore.cc's napi_get_value_string_utf16 / latin1 / utf8 getters now short-circuit bufsize == 0 before delegating to the copy helper (mirroring the Chakra fix). V8 already handles bufsize == 0 correctly, so the test now runs on Chakra, V8, and JavaScriptCore.
napi_get_value_string_utf16 computed bufsize - 1 as the destination capacity and stored the null terminator at buf[bufsize - 1]. With a non-null buffer and bufsize == 0, bufsize - 1 underflows to SIZE_MAX, so the entire JS string is copied into the zero-length buffer and the terminator is written at buf[SIZE_MAX] -- an attacker-sized out-of-bounds write reachable from any native caller passing (buf != nullptr, bufsize == 0) (CWE-191 -> CWE-787). This affected both the Chakra and JavaScriptCore backends: - Chakra (js_native_api_chakra.cc): gate the copy on bufsize != 0 (mirroring the upstream Node implementation). The latin1/utf8 siblings shared a milder bug -- with bufsize == 0 they took the slow path (a needless allocation) and stored the terminator at buf[0], one byte past a zero-length buffer; both now short-circuit bufsize == 0 too. - JavaScriptCore (js_native_api_javascriptcore.cc): the utf16/latin1/utf8 getters delegate to JSString::CopyTo*, which underflow bufsize - 1 and always write buf[size] = 0. Short-circuit bufsize == 0 at the getter before calling the copy helper. V8 already handles bufsize == 0 correctly and is unchanged. Add a native regression test (this path is not reachable from JS) that calls napi_get_value_string_utf16 with a sentinel buffer and bufsize == 0, asserting nothing is written and the reported length is zero, while a normally-sized buffer still copies and null-terminates. Runs on Chakra, V8 and JavaScriptCore; guarded off the V8JSI backend, which does not expose napi_get_value_string_utf16. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bkaradzic-microsoft
force-pushed
the
fix/string-utf16-bufsize-underflow
branch
from
063b9db to
e7682b1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes an integer underflow in the Chakra Node-API string getters when a caller passes a non-null buffer with
bufsize == 0.napi_get_value_string_utf16forwardedbufsize - 1toJsCopyStringUtf16as the destination capacity and stored the null terminator atbuf[bufsize - 1]. Withbufsize == 0,bufsize - 1underflows toSIZE_MAX, so:buf[SIZE_MAX].That's an attacker-sized out-of-bounds write reachable from any native caller that passes
(buf != nullptr, bufsize == 0)(CWE-191 → CWE-787/CWE-120).Fix
napi_get_value_string_utf16: gate the copy onbufsize != 0(mirroring the upstream Node implementation). A non-null buffer withbufsize == 0now reports zero copied and writes nothing.napi_get_value_string_latin1/napi_get_value_string_utf8: these shared a related, milder bug — withbufsize == 0they took the slow path (a needless allocation) and stored the terminator atbuf[0], one byte past a zero-length buffer. Both now short-circuitbufsize == 0identically.Test
Adds a native regression test (
NodeApi.GetValueStringUtf16HandlesZeroBufsize) — the path isn't reachable from JS, so it's exercised through the C API. It passes a sentinel-filled buffer withbufsize == 0and asserts nothing is written and the reported length is zero, while a normally-sized buffer still copies and null-terminates. Guarded off the V8JSI backend (which doesn't exposenapi_get_value_string_utf16), same as thenapi_create_dataviewregression test from #181.Verified (Win32/Chakra): builds clean; the new test and the full UnitTests suite pass.